Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flytepropeller][compiler] Error Handling when Type is not found #5612

Merged

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Aug 1, 2024

Tracking issue

https://linear.app/unionai/issue/DX-948/flytepropeller-compiler-error-handling-when-type-is-not-found

Why are the changes needed?

When developing JSON IDL, I found that if I registered task to flytepropeller but flytepropeller didn't support the corresponding flyteidl, flyteadmin will crash and the error message from flyteadmin doesn't have a clear error message.

Before

flytekit

image

flytepropeller

image

After

We should give a good error message and avoid crashing the propeller.

image

What changes were proposed in this pull request?

Improve error message when inputType is nil, which means flytepropeller didn't support that type.

How was this patch tested?

single binary and unit test.

Setup process

flytekit: flyteorg/flytekit#2600
flyteidl: #5542
flyte: this branch

from flytekit import task, workflow

@task()
def t5(input: dict) -> dict:
    print(input)
    print("different")
    return input.update({"b": 2}).update({"c": 3})

@workflow
def wf2(input: dict={'d': 1, }) -> dict:
    return t5(input=input)
pyflyte run --remote build/PR/JSON/example.py wf2

Screenshots

image

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

#5542

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.17%. Comparing base (025296a) to head (4cdce3e).
Report is 129 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5612      +/-   ##
==========================================
+ Coverage   35.89%   36.17%   +0.28%     
==========================================
  Files        1301     1302       +1     
  Lines      109419   109417       -2     
==========================================
+ Hits        39281    39587     +306     
+ Misses      66041    65694     -347     
- Partials     4097     4136      +39     
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.34% <100.00%> (+1.64%) ⬆️
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.28% <ø> (ø)
unittests-flyteidl 7.09% <ø> (ø)
unittests-flyteplugins 53.31% <ø> (ø)
unittests-flytepropeller 41.73% <ø> (-0.03%) ⬇️
unittests-flytestdlib 55.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Future-Outlier Future-Outlier changed the title [flytepropeller][compiler] Avoids crashing when a type is not found. [flytepropeller][compiler] Avoid crashing when a type is not found Aug 1, 2024
@Future-Outlier Future-Outlier changed the title [flytepropeller][compiler] Avoid crashing when a type is not found [flytepropeller][compiler] Error Handling when Type is not found Aug 1, 2024
Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>
@eapolinario
Copy link
Contributor

@Future-Outlier , can you fix the lint error?

@wild-endeavor
Copy link
Contributor

@Future-Outlier just wondering does this fail in the same way for pyflyte register? or is it just run?

@Future-Outlier
Copy link
Member Author

@Future-Outlier just wondering does this fail in the same way for pyflyte register? or is it just run?

Both failed
They will go through the same code piece

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
@Future-Outlier Future-Outlier enabled auto-merge (squash) August 2, 2024 00:55
Signed-off-by: Future-Outlier <[email protected]>
@Future-Outlier Future-Outlier merged commit 17719e2 into master Aug 2, 2024
49 of 50 checks passed
@Future-Outlier Future-Outlier deleted the flytepropeller-compiler-type-not-found-error-message branch August 2, 2024 06:24
bgedik pushed a commit to bgedik/flyte that referenced this pull request Aug 15, 2024
…teorg#5612)

* FlytePropeller Compiler Avoid Crash when Type not found

Signed-off-by: Future-Outlier <[email protected]>

* Update pingsu's error message advices

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>

* fix lint

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>
vlibov pushed a commit to vlibov/flyte that referenced this pull request Aug 16, 2024
…teorg#5612)

* FlytePropeller Compiler Avoid Crash when Type not found

Signed-off-by: Future-Outlier <[email protected]>

* Update pingsu's error message advices

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>

* fix lint

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Signed-off-by: Vladyslav Libov <[email protected]>
eapolinario added a commit that referenced this pull request Aug 20, 2024
…ame (#5616)

* Add environment variable for pod name

Signed-off-by: Bugra Gedik <[email protected]>

* [flyteadmin] Refactor panic recovery into middleware (#5546)

* Refactor panic handling to middleware

Signed-off-by: Jason Parraga <[email protected]>

* Remove registration of old panicCounter

Signed-off-by: Jason Parraga <[email protected]>

* Add test coverage

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Snowflake agent Doc (#5620)

* TEST build

Signed-off-by: Future-Outlier <[email protected]>

* remove emphasize-lines

Signed-off-by: Future-Outlier <[email protected]>

* test build

Signed-off-by: Future-Outlier <[email protected]>

* revert

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* [flytepropeller][compiler] Error Handling when Type is not found (#5612)

* FlytePropeller Compiler Avoid Crash when Type not found

Signed-off-by: Future-Outlier <[email protected]>

* Update pingsu's error message advices

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>

* fix lint

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Fix nil pointer when task plugin load returns error (#5622)

Signed-off-by: Bugra Gedik <[email protected]>

* Log stack trace when refresh cache sync recovers from panic (#5623)

Signed-off-by: Bugra Gedik <[email protected]>

* use private-key (#5626)

Signed-off-by: Bugra Gedik <[email protected]>

* Explain how Agent Secret Works (#5625)

* first version

Signed-off-by: Future-Outlier <[email protected]>

* update

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Fix typo in execution manager (#5619)

Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Amend Admin to use grpc message size (#5628)

* add send arg

Signed-off-by: Yee Hing Tong <[email protected]>

* Add acction to remove cache in gh runner

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use correct checked out path

Signed-off-by: Eduardo Apolinario <[email protected]>

* Path in strings

Signed-off-by: Eduardo Apolinario <[email protected]>

* Checkout repo in root

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use the correct path to new action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Do not use gh var in path to clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove wrong invocation of clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* GITHUB_WORKSPACE is implicit in the checkout action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Refer to local `flyte` directory

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* document the process of setting ttl for a ray cluster (#5636)

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Add CustomHeaderMatcher to pass additional headers (#5563)

Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Turn flyteidl and flytectl releases into manual gh workflows (#5635)

* Make flyteidl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Make flytectl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Rewrite the documentation for `version` and clarify wording in RELEASE.md

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* docs: fix typo (#5643)

* fix CHANGELOG-v0.2.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.0.2-b1.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.1.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.3.0.md

Signed-off-by: Christina <[email protected]>

---------

Signed-off-by: Christina <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Use enable_deck=True in docs (#5645)

Signed-off-by: Bugra Gedik <[email protected]>

* Fix flyteidl release  checkout all tags (#5646)

* Fetch all tags in flyteidl-release.yml

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix sed expression for npm job

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Install pyarrow in sandbox functional tests (#5647)

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* docs: add documentation for configuring notifications in GCP (#5545)

* update

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* typo

Signed-off-by: Desi Hsu <[email protected]>

---------

Signed-off-by: Desi Hsu <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Correct "sucessfile" to "successfile" (#5652)

Signed-off-by: Bugra Gedik <[email protected]>

* Fix ordering for custom template values in cluster resource controller (#5648)

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Don't error when attempting to trigger schedules for inactive projects (#5649)

* Don't error when attempting to trigger schedules for inactive projects

Signed-off-by: Katrina Rogan <[email protected]>

* regen

Signed-off-by: Katrina Rogan <[email protected]>

---------

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* fix tests

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* Fix comment symbol

Signed-off-by: Eduardo Apolinario <[email protected]>

* fix one more test

Signed-off-by: Bugra Gedik <[email protected]>

---------

Signed-off-by: Bugra Gedik <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Christina <[email protected]>
Signed-off-by: Desi Hsu <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Co-authored-by: Jason Parraga <[email protected]>
Co-authored-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Co-authored-by: ddl-rliu <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Andrew Dye <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Christina <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: desihsu <[email protected]>
Co-authored-by: ShengYu <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
@eapolinario eapolinario mentioned this pull request Sep 4, 2024
3 tasks
pmahindrakar-oss pushed a commit that referenced this pull request Sep 9, 2024
…ame (#5616)

* Add environment variable for pod name

Signed-off-by: Bugra Gedik <[email protected]>

* [flyteadmin] Refactor panic recovery into middleware (#5546)

* Refactor panic handling to middleware

Signed-off-by: Jason Parraga <[email protected]>

* Remove registration of old panicCounter

Signed-off-by: Jason Parraga <[email protected]>

* Add test coverage

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Snowflake agent Doc (#5620)

* TEST build

Signed-off-by: Future-Outlier <[email protected]>

* remove emphasize-lines

Signed-off-by: Future-Outlier <[email protected]>

* test build

Signed-off-by: Future-Outlier <[email protected]>

* revert

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* [flytepropeller][compiler] Error Handling when Type is not found (#5612)

* FlytePropeller Compiler Avoid Crash when Type not found

Signed-off-by: Future-Outlier <[email protected]>

* Update pingsu's error message advices

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw  <[email protected]>

* fix lint

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

* Trigger CI

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Fix nil pointer when task plugin load returns error (#5622)

Signed-off-by: Bugra Gedik <[email protected]>

* Log stack trace when refresh cache sync recovers from panic (#5623)

Signed-off-by: Bugra Gedik <[email protected]>

* use private-key (#5626)

Signed-off-by: Bugra Gedik <[email protected]>

* Explain how Agent Secret Works (#5625)

* first version

Signed-off-by: Future-Outlier <[email protected]>

* update

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Fix typo in execution manager (#5619)

Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Amend Admin to use grpc message size (#5628)

* add send arg

Signed-off-by: Yee Hing Tong <[email protected]>

* Add acction to remove cache in gh runner

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use correct checked out path

Signed-off-by: Eduardo Apolinario <[email protected]>

* Path in strings

Signed-off-by: Eduardo Apolinario <[email protected]>

* Checkout repo in root

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use the correct path to new action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Do not use gh var in path to clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove wrong invocation of clear-action-cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* GITHUB_WORKSPACE is implicit in the checkout action

Signed-off-by: Eduardo Apolinario <[email protected]>

* Refer to local `flyte` directory

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* document the process of setting ttl for a ray cluster (#5636)

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Add CustomHeaderMatcher to pass additional headers (#5563)

Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Turn flyteidl and flytectl releases into manual gh workflows (#5635)

* Make flyteidl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Make flytectl releases go through a manual gh workflow

Signed-off-by: Eduardo Apolinario <[email protected]>

* Rewrite the documentation for `version` and clarify wording in RELEASE.md

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* docs: fix typo (#5643)

* fix CHANGELOG-v0.2.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.0.2-b1.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.1.0.md

Signed-off-by: Christina <[email protected]>

* fix CHANGELOG-v1.3.0.md

Signed-off-by: Christina <[email protected]>

---------

Signed-off-by: Christina <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Use enable_deck=True in docs (#5645)

Signed-off-by: Bugra Gedik <[email protected]>

* Fix flyteidl release  checkout all tags (#5646)

* Fetch all tags in flyteidl-release.yml

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix sed expression for npm job

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Install pyarrow in sandbox functional tests (#5647)

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* docs: add documentation for configuring notifications in GCP (#5545)

* update

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* dco

Signed-off-by: Desi Hsu <[email protected]>

* typo

Signed-off-by: Desi Hsu <[email protected]>

---------

Signed-off-by: Desi Hsu <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Correct "sucessfile" to "successfile" (#5652)

Signed-off-by: Bugra Gedik <[email protected]>

* Fix ordering for custom template values in cluster resource controller (#5648)

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* Don't error when attempting to trigger schedules for inactive projects (#5649)

* Don't error when attempting to trigger schedules for inactive projects

Signed-off-by: Katrina Rogan <[email protected]>

* regen

Signed-off-by: Katrina Rogan <[email protected]>

---------

Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>

* fix tests

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* change to shorter names

Signed-off-by: Bugra Gedik <[email protected]>

* Fix comment symbol

Signed-off-by: Eduardo Apolinario <[email protected]>

* fix one more test

Signed-off-by: Bugra Gedik <[email protected]>

---------

Signed-off-by: Bugra Gedik <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Andrew Dye <[email protected]>
Signed-off-by: Christina <[email protected]>
Signed-off-by: Desi Hsu <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Co-authored-by: Jason Parraga <[email protected]>
Co-authored-by: Future-Outlier <[email protected]>
Co-authored-by: pingsutw <[email protected]>
Co-authored-by: ddl-rliu <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Andrew Dye <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Christina <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: desihsu <[email protected]>
Co-authored-by: ShengYu <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
Signed-off-by: pmahindrakar-oss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants